Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

open-mpi: Use new Apple linker for mpicc/mpicxx #171426

Merged
merged 6 commits into from
May 17, 2024

Conversation

lukeshingles
Copy link
Contributor

A flag to force the ld_classic linker was added in #144554 to avoid issues with Xcode 15. However, some time between then and now (Xcode 15.3 and macOS Sonoma 14.4.1) the new linker seems to work for C/C++, although the Fortran test still fails when ld_new is used. In this PR, I have limited the ld_classic flag to only apply to mpifort wrapper compiler. My motivation for switching to the new linker for C++ is that it solves linker failures when I try to compile my C++ project with mpicxx -flto=auto and g++-14 as the host compiler.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the CI-linux-self-hosted Build on Linux self-hosted runner label May 11, 2024
@fxcoudert
Copy link
Member

I'm wondering if we shouldn't keep the linker consistent, in particular when linking both C and Fortran (but with a main in C).

@lukeshingles
Copy link
Contributor Author

I'm wondering if we shouldn't keep the linker consistent, in particular when linking both C and Fortran (but with a main in C).

This seems to work, at least for a simple test (is there an open-source C/Fortran project I should test?).

fortsub.f90

subroutine myfortsub (  ) bind ( C, name="myfortsub" )
   use mpi
   implicit none

    integer rank, size, ierror, tag, status(MPI_STATUS_SIZE)
    ! call MPI_INIT(ierror)
    call MPI_COMM_SIZE(MPI_COMM_WORLD, size, ierror)
    call MPI_COMM_RANK(MPI_COMM_WORLD, rank, ierror)
    print*, 'node', rank, ': Hello from Fortran'
    ! call MPI_FINALIZE(ierror)
   return
end subroutine myfortsub

main.c

#include <mpi.h>
#include <stdio.h>

extern void myfortsub();

int main() {
  MPI_Init(NULL, NULL);
  myfortsub();
  int size, rank, nameLen;
  char name[MPI_MAX_PROCESSOR_NAME];
  MPI_Comm_size(MPI_COMM_WORLD, &size);
  MPI_Comm_rank(MPI_COMM_WORLD, &rank);
  MPI_Get_processor_name(name, &nameLen);
  printf("[%d/%d] Hello from C! My name is %s.\n", rank, size, name);
  MPI_Finalize();
  return 0;
}

Build and link:

mpifort -c fortsub.f90
mpicc -c main.c
mpifort fortsub.o main.o -o hello
mpirun -np 8 ./hello

Output:

ld: warning: -commons use_dylibs is no longer supported, using error treatment instead
node           3 : Hello from Fortran
 node           2 : Hello from Fortran
[2/4] Hello from C! My name is Luke-MBP2021.fritz.box.
[3/4] Hello from C! My name is Luke-MBP2021.fritz.box.
 node           1 : Hello from Fortran
[1/4] Hello from C! My name is Luke-MBP2021.fritz.box.
 node           0 : Hello from Fortran
[0/4] Hello from C! My name is Luke-MBP2021.fritz.box.

The linker warning also occurs with the previous version of the forumula.

@lukeshingles
Copy link
Contributor Author

Sorry to invalidate the review. I forgot to bump the formula revision (which I think is necessary?).

Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@lukeshingles
Copy link
Contributor Author

Newbie here, so I don't really understand the procedures. I can locally reproduce the errors with opencoarrays and vineyard, and they are fixed when I brew install --build-from-source. Does this mean I should bump the revisions of these forumulae in my PR to get new bottles built?

The error with root test is the same (at least locally) whether I use the modified open-mpi formula from this PR or the existing one. My only guess is that the failure could be related to an Xcode update. Any ideas?

@Bo98
Copy link
Member

Bo98 commented May 13, 2024

Does this mean I should bump the revisions of these forumulae in my PR to get new bottles built?

Yes for opencoarrays and vineyard. Ideally they'd also be fixed to not need a revision bump every time open-mpi is revision bumped but just revision bumping them is OK for now.

root is probably Xcode update so can ignore that one.

@Bo98 Bo98 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label May 13, 2024
@lukeshingles
Copy link
Contributor Author

I'm afraid I don't understand the brew linkage errors. Are these indicating real issues caused by the changes to the open-mpi formula here?

Copy link
Contributor

Copy link
Contributor

⚠️ @carlocab bottle publish failed.

@carlocab carlocab added CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. and removed CI-linux-self-hosted Build on Linux self-hosted runner labels May 16, 2024
@carlocab
Copy link
Member

Looks like you need to rebase on master.

@github-actions github-actions bot added the CI-linux-self-hosted Build on Linux self-hosted runner label May 17, 2024
@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request autosquash Automatically squash pull request commits according to Homebrew style. autobump and removed autobump labels May 17, 2024
@github-actions github-actions bot removed automerge-skip `brew pr-automerge` will skip this pull request autosquash Automatically squash pull request commits according to Homebrew style. labels May 17, 2024
@lukeshingles lukeshingles force-pushed the open-mpi-ldnew branch 2 times, most recently from b7adf39 to 7250ffa Compare May 17, 2024 15:00
A flag to force the ld_classic linker was added in Homebrew#144554 to avoid issues with Xcode 15. However, some time between then and now (Xcode 15.3 and macOS Sonoma 14.4.1) the new linker seems to work for C/C++, although the Fortran test still fails when ld_new is used. In this PR, I have limited the ld_classic flag to only apply to mpifort wrapper compiler. My motivation for switching to the new linker for C++ is that it solves linker failures when I try to compile my C++ project with mpicxx -flto=auto and g++-14 as the host compiler.
@carlocab carlocab added CI-linux-self-hosted Build on Linux self-hosted runner CI-skip-dependents Pass --skip-dependents to brew test-bot. and removed CI-linux-self-hosted Build on Linux self-hosted runner labels May 17, 2024
Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label May 17, 2024
@github-actions github-actions bot added the CI-linux-self-hosted Build on Linux self-hosted runner label May 17, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue May 17, 2024
Merged via the queue into Homebrew:master with commit 39a0f83 May 17, 2024
14 checks passed
carlocab added a commit that referenced this pull request May 17, 2024
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. CI-skip-dependents Pass --skip-dependents to brew test-bot. CI-skip-recursive-dependents Pass --skip-recursive-dependents to brew test-bot. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants